Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow reading data files bundled within theme-gems #5470

Closed
wants to merge 11 commits into from

Conversation

ashmaroli
Copy link
Member

@ashmaroli ashmaroli commented Oct 9, 2016

If a theme gem contains a _data folder at its root, its files will be read and added to the site's internal data hash. The theme-gem's data hash will be overridden by data at source if the same 'key-value' pair exists there.

TODO:

  • Add tests
  • Add cucumber feature
  • Add Documentation

Ref: #5434
/cc @jekyll/ecosystem, @envygeeks, @pathawks

@pathawks
Copy link
Member

pathawks commented Oct 9, 2016

For what purpose? Seems like a violation of separation of design from content.

I feel like there must be a better way to do i18n.

@ashmaroli
Copy link
Member Author

ashmaroli commented Oct 9, 2016

For what purpose?

Its not just for i18n per se.. but having this can lead to creating a more functional theme-gem.. I dont have a solid use case for this as of now.. will be fleshed out it in the referenced ticket (no. 5434)

Seems like a violation of separation of design from content.

No.. not all..
though my original intention was to use it for structural elements like define a navigation system using navigation.yml , etc.. I now see the possibilities for built-in i18n as well..

@benbalter
Copy link
Contributor

@ashmaroli thanks for opening up this PR. I really like the vision for themes that you laid out in #5434.

I dont have a solid use case for this as of now.

Before we move forward, I'd feel a lot more comfortable if we understood why we were building the feature, rather than just building it for a sense of symetry with support for other Jekyll-specific folders, or because it's easy to do so. As was mentioned over in #5350, I share @pathawks concerns that themes should only know about presentation, not content (to ensure theme fungability).

We really, really need to figure out a solid I18N implemntation, but I don't know that _data folder support for themes is it.

@ghost
Copy link

ghost commented Oct 10, 2016

No.. not all..
though my original intention was to use it for structural elements like define a navigation system using navigation.yml , etc.. I now see the possibilities for built-in i18n as well..

Seems like a good use case... for when you are using <site root>/_data/navigation.yml (which already works), not <theme root>/_data/navigation.yml 😄

@ashmaroli
Copy link
Member Author

I share Pat Hawks' concerns that themes should only know about presentation, not content (to ensure theme fungibility).

I care about theme fungibility too and this feature does not mess with content in the manner you guys fear. Content, to me, comes from markdown files at source, supported by files in an optional _data folder.

@oncleben31
Copy link

I feel like there must be a better way to do i18n.

We really, really need to figure out a solid I18N implemntation,

Have you some guideline as I'm trying to add basic i18n features to minima template (in jekyll/minima#60) ?
Currently I feel the best option is to use locale string file in _data folder. But I'm new to Jekyll so if you have better ideas please share it.

If a theme gem contains a '_data' folder at its root, its files will be
read and added to the site's internal data hash.
The theme-gem's data hash will be overridden by data at source if the same
'key-value' pair exists there.
@dummied
Copy link

dummied commented Nov 13, 2016

Did #5491 (and ashmaroli/jekyll-data) render this moot?

@ashmaroli
Copy link
Member Author

@dummied This is still active, provided there's a demand for this to be included in the core

@Swiftrien
Copy link

I have a theme in which I provide default values for configuration and theme-translation via the _data directory.
If not in the _data directory, how else could I provide default values?

@ashmaroli
Copy link
Member Author

@Swiftrien you may also give the jekyll-data gem a try in the meantime..
It provides the same functionality as this pull, and a li'l bit more.. 😃

@Swiftrien
Copy link

Thanks ashmaroli

However that would mean that every user of my theme also has to install it. Thus this is not the answer to this issue..

@ashmaroli
Copy link
Member Author

However that would mean that every user of my theme also has to install it.

@Swiftrien I'm not trying to promote a plugin I authored, but if you add jekyll-data as a runtime_dependency to your theme's gemspec, it will be automatically installed with your theme-gem.
Its entirely your call. I won't mind if you decide otherwise.

But I'd definitely like it if you gave it a try, and provide a feedback.
I could then update this pull based on that.

Either way, Happy Jekylling 😃

@Swiftrien
Copy link

Swiftrien commented Nov 21, 2016

@ashmaroli
I have nothing against promotion :-)
classic-jekyll-theme :-)

I just added the jekyll-data (0.3.0) gem to a website of mine, removed the _data directory, rebuild the site, and everything is as it should be. Thus jekyll-data seems to work fine. Note that I did not use the "theme configuration" only the "regular data" files.

My bigger issue with external contributions is that it takes away my control. And I have had several experiences where external dependencies costs me huge amounts of time. So in general I try to avoid them where possible. As it is, I think that the jekyll-data gem adds enough value that I will probably include it in the next release. (Mainly because I hope it will be part of Jekyll 3.4 anyhow... ;-))

@DirtyF DirtyF unassigned parkr Nov 8, 2018
@Ryuno-Ki
Copy link
Contributor

Ryuno-Ki commented Nov 8, 2018

@ellemenno As @mattr- wrote in #5470 (comment)

Failing builds + conflicts in the first place :-)

@ashmaroli ashmaroli changed the title Add ThemeDataReader for data files in theme-gems Allow reading data files bundled within theme-gems Aug 25, 2019
@mattr- mattr- added this to In progress in Jekyll 4.1 via automation Aug 26, 2019
@mattr- mattr- added this to the 4.1 milestone Aug 26, 2019
@mattr-
Copy link
Member

mattr- commented Aug 26, 2019

Scheduling this for inclusion in 4.1

@ashmaroli
Copy link
Member Author

Scheduling this for inclusion in 4.1

That's awesome news @mattr-. Thank you.

@DirtyF DirtyF moved this from In progress to In Review in Jekyll 4.1 Mar 17, 2020
Copy link
Member

@mattr- mattr- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One tiny nit in a comment but ok otherwise.

lib/jekyll/reader.rb Outdated Show resolved Hide resolved
Co-Authored-By: Matt Rogers <mattr-@github.com>
@DirtyF
Copy link
Member

DirtyF commented Apr 13, 2020

Waiting for the tests to pass to merge...

@ashmaroli
Copy link
Member Author

In light of recently acquired knowledge that unlike layouts or includes which are overridden entirely by the namesake at source, data read-in may at times persist partially warranting additional user-intervention to override the persisting data, I'm withdrawing this submission.

Those interested in this functionality may opt to use the jekyll-data plugin instead.

@ashmaroli ashmaroli closed this Apr 30, 2020
@ashmaroli ashmaroli deleted the theme-data branch April 30, 2020 09:21
@ashmaroli ashmaroli removed this from the 4.1 milestone Apr 30, 2020
@yatil
Copy link

yatil commented Apr 30, 2020

unlike layouts or includes which are overridden entirely by the namesake at source, data read-in may at times persist partially warranting additional user-intervention to override the persisting data

Is this something that is inherit to the technology or the result of a decision in Jekyll. And if the latter, can this decision be changed? I think it is hard to understand for users that you can have templating in a theme for everything but not for _data files.

@ashmaroli
Copy link
Member Author

@yatil While I understand the sentiment, please consider the following scenario:
A theme has bundled the following data file named social_links.yml:

twitter: jekyllrb
github:  jekyll

And the theme uses the following HTML include to render social media icons:

{%- assign social = site.data.social_links -%}
<ul class="social-media-list">
  {% if social.github %}<li><a href="https://github.com/{{ social.github }}" title="{{ social.github  }}"><svg class="svg-icon"><use xlink:href="{{ '/assets/imgs/social-icons.svg#github' | relative_url }}"></use></svg></a></li>{% endif %}
  {% if social.twitter %}<li><a href="https://twitter.com/{{ social.twitter }}" title="{{ social.twitter  }}"><svg class="svg-icon"><use xlink:href="{{ '/assets/imgs/social-icons.svg#twitter' | relative_url }}"></use></svg></a></li>{%- endif -%}
{% if social.linkedin %}<li><a  href="https://www.linkedin.com/in/{{ social.linkedin  }}" title="{{ social.linkedin }}"><svg class="svg-icon"><use xlink:href="{{ '/assets/imgs/social-icons.svg#linkedin' | relative_url }}"></use></svg></a></li>{% endif %}
  <!-- ... so on ... -->
</ul>

Now, suppose one of the theme's user doesn't have a GitHub account or a Twitter handle.. they think that all they need to do is override the theme's data file with one at their source:

# source/_data/social_links.yml

linkedin: johndoe

However, when they build their site, all three social icons are rendered by default.
The user has to then explicitly set github and twitter keys to null to not render them, A couple of keys may not be a hassle. But imagine if the theme supports over 50 social accounts.., the user would have to manually disable over 4 dozens of keys...........

@yatil
Copy link

yatil commented May 1, 2020

@ashmaroli

However, when they build their site, all three social icons are rendered by default.

Because both, the theme's social_links.yml and the site’s social_links.yml are interpreted. But, theoretically, wouldn’t it be possible to say “if the site has a social_links.yml, ignore the theme’s social_links.yml”?

I personally can see both behaviors to be beneficial, but one would then need to be able to configure it and that might create problems.

Thanks for exploring the idea, it is much appreciated.

@ashmaroli
Copy link
Member Author

would then need to be able to configure it and that might create problems

Jekyll 4.0 introduced the ability to import configuration from a theme-gem and merge it with the one from the source. It led to problems similar to what I described above.
And now Jekyll 4.1 will make amends by providing users with an option to disable loading the configuration from the theme.

theoretically, wouldn’t it be possible to say “if the site has a social_links.yml, ignore the theme’s social_links.yml”?

Yes, that is possible. I had considered that but there's catch though.
_data/foo.yml containing top-level key bar: and _data/foo/bar.yml are equivalent and it is not possible to know about top-level keys without parsing the data file first.

@jekyll jekyll locked and limited conversation to collaborators May 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Jekyll 4.1
  
In Review
Development

Successfully merging this pull request may close these issues.

None yet